fix(engine): report the cause and attempt count when a node exhausts retries - #77

Summary

node failed after retries was emitted with the node id and nothing else:

tracing::warn!(node = %node.id, "node failed after retries");

That is the only line a run produces when a node gives up, so a report of it cannot be root-caused — the log says a node failed, but not why, and not how many attempts it took. This is exactly what happened to tinyhumansai/openhuman#5626, which is a single staging occurrence of this warning and cannot be diagnosed as filed.

The cause was already in hand and simply not named. last_err: Option<EngineError> is a parameter of finish_execution and is in scope at the warning; it is just not unwrapped until the let Some(err) 44 lines further down. This borrows it there (the unwrap below still takes it by value), adds the on_error policy that decides what happens next, and threads attempts_used through from the retry loop so "after retries" carries a number.

tracing::warn!(
    node = %node.id,
    attempts,
    on_error = %on_error,
    error = ?last_err,
    "node failed after retries"
);

Deliberately logged at the existing site rather than after the unwrap: the None arm below it is the defensive unreachable path, and moving the line past it would stop that case reporting at all.

What this is not

This is a diagnostic prerequisite, not a root-cause fix. It does not explain why any particular node failed, and it is not expected to change any observed behaviour. tinyhumansai/openhuman#5626 needs the workflow graph containing the node and a staging log window to go further, neither of which exists in a repository — that issue should stay open, and closing it needs a person, not this PR.

Worth stating for anyone reading the linked issue: [outcome] is not a subsystem and there is no product feature called "the summarize outcome node". [outcome] is the tracing target derived from this module's path — the generic node-completion handler every node passes through — and summarize is just an id a graph author chose.

API Or Behavior Changes

None to any public API. finish_execution is pub(super) and gains one parameter; its only caller is engine/build/activation.rs, updated here.

Behaviour is log-only: no control flow is touched. The failure was already routed independently of this line — the ExecutionStep { status: StepStatus::Error, .. } pushed just below it, observer.on_step_finish, and the continue/route/stop policy after it are all unchanged.

One thing for a reviewer to weigh: EngineError can carry tool and config text, so this is a new place where payload fragments may reach logs. It is at WARN on a path that only runs after every retry has failed, but it is a real change in what gets written, and every downstream consumer inherits it on the next bump.

Tests

New: tests/node_failure_diagnostics_e2e.rs. It registers a hand-rolled global tracing::Subscriber that keeps every event with its fields keyed by name, runs a two-node graph whose tool_call has no slug (the deterministic Capability failure tests/error_recovery_e2e.rs already uses) with retry.max_attempts: 3, and asserts the warning carries node, error, attempts = 3 and on_error = stop.

Two deliberate choices:

• No new dependency. The subscriber is ~50 lines against tracing itself rather than pulling in tracing-subscriber; this crate takes tracing with default-features = false and carries no subscriber dep. Registered globally rather than via with_default because the engine runs on tokio worker threads and a thread-local dispatcher would miss them — each tests/*.rs is its own binary, so the one global registration is safe.
• Fields are looked up by name, so the test is not vacuous. Asserting that error merely renders non-empty would still pass if the field were dropped again, because a missing field and an empty one are indistinguishable once formatted.

Proven to fail without the fix. Reverting the warning to its previous form and keeping the test:

---- retries_exhausted_warning_reports_the_cause_and_the_attempt_count stdout ----
thread '...' panicked at tests/node_failure_diagnostics_e2e.rs:180:9:
the retries-exhausted warning carries no `error` field, so nothing in the log
can say why the node failed; fields were {"node": "summarize"}

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

fields were {"node": "summarize"} is the whole problem in one line.

Commands run locally on this branch, all exit 0:

• cargo fmt --check
• cargo clippy --all-targets -- -D warnings
• cargo clippy --all-targets --all-features -- -D warnings
• cargo build --all-targets
• cargo build --all-targets --all-features
• cargo test — 1376 passed, 0 failed
• cargo test --all-features — 1421 passed, 0 failed

The full suite matters here rather than a scoped filter: finish_execution's signature changed, so every caller and every test that drives a failing node had to be re-checked, not just the new file.

Documentation

None needed — the rationale for logging at this site rather than after the unwrap is a comment at the call site, where the next person to move the line will read it.

Summary by CodeRabbit

• Bug Fixes

  • Improved diagnostics when a node fails after exhausting retries.
  • Retry warnings now identify the failed node, number of attempts, configured error-handling policy, and underlying error.

• Tests

  • Added end-to-end coverage verifying retry-failure details are reported correctly.